Skip to content

Default Regex Filter Matchers, Job Argument Filtering, Reserved Keys Protection, SQL Bind Parameter Filtering #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ndbroadbent merged 4 commits into from
Jan 30, 2026
Merged

Default Regex Filter Matchers, Job Argument Filtering, Reserved Keys Protection, SQL Bind Parameter Filtering #5

ndbroadbent merged 4 commits into from
Jan 30, 2026

Conversation

@ndbroadbent
Copy link
Member

@ndbroadbent ndbroadbent commented Jan 29, 2026 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • New Features

    • Job arguments are logged by default unless a job class opts out.
    • Improved sensitive-data filtering: JWTs, Bearer tokens, and a default regex for common secret/key patterns are now detected and filtered.
    • Core log fields (src, evt, lvl, ts) are protected from being overwritten by additional data.

  • Tests

    • Added tests for argument-logging behavior, token filtering, and reserved-field protection.

  • Chores

    • Test tooling updated to install gems locally and run tasks within the bundler environment.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026 •
edited
Loading

Caution

Review failed

The pull request is closed.

Walkthrough

Adds job-level argument logging control (via log_arguments?), expands sensitive-data detection to include JWT and Bearer tokens and a default sensitive-key regex, and prevents additional_data from overwriting reserved log fields (src, evt, lvl, ts). Tests and scripts updated accordingly.

Changes

Cohort / File(s) Summary
Argument logging (ActiveJob / GoodJob)
lib/log_struct/builders/active_job.rb, lib/log_struct/integrations/good_job/log_subscriber.rb, test/log_struct/integrations/good_job/log_subscriber_test.rb		 Introduce safe_arguments(job) to centralize whether job arguments are logged; default to logging unless the job class defines log_arguments? returning false. Add tests for opt-in/opt-out behaviors.
Sensitive-key pattern & param filtering
lib/log_struct/config_struct/filters.rb, test/log_struct/param_filters_test.rb		 Add DEFAULT_SENSITIVE_KEY_PATTERN regex and include a new FilterMatcher using it. Add tests to verify common sensitive keys are matched and reduce false positives.
ActiveRecord token detection
lib/log_struct/integrations/active_record.rb, test/log_struct/integrations/active_record_test.rb		 Enhance looks_sensitive? to detect JWT-like (header.payload.signature starting with ey) and Bearer tokens; add tests ensuring JWT/Bearer tokens in SQL bind params are filtered.
Reserved key protection in merges
lib/log_struct/shared/merge_additional_data_fields.rb, test/log_struct/integrations/lograge_formatter_test.rb		 Add RESERVED_KEYS (:src, :evt, :lvl, :ts) and guard merge logic to raise/log ArgumentError when additional_data attempts to overwrite reserved fields. Add test ensuring exceptions are reported and original values retained.
CI / scripts
rails_test_app/create_app.rb, scripts/rails_tests.sh, scripts/merge_coverage.sh		 Configure local BUNDLE_PATH for Rails test app and prefix coverage merge with bundle exec to run tasks in Bundler environment.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 In burrows of logs I quietly hop,
I hide JWTs and Bearer strings nonstop,
I guard the keys: src, evt, lvl, ts — no theft,
Arguments show only when jobs have left their clef,
Hooray for safer, snugger logcraft! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title comprehensively covers all four major changes: default regex filter matchers, job argument filtering, reserved keys protection, and SQL bind parameter filtering, accurately reflecting the scope of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch nathan/security-fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

ndbroadbent and others added 3 commits January 30, 2026 13:48
@ndbroadbent
fix duplication
886cbb8
@ndbroadbent @claude
Fix Ruby 4.0 bundler security error on CI
76cc71f 
Ruby 4.0 bundler rejects reinstalling gems in world-writable directories
for security reasons. Set BUNDLE_PATH to vendor/bundle in the Rails test
app to avoid using the system gems directory on CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ndbroadbent @claude
Fix merge_coverage.sh to use bundle exec
c233657 
The rake command needs to run through bundler to find simplecov.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ndbroadbent ndbroadbent merged commit d4037c8 into main Jan 30, 2026
7 checks passed
@ndbroadbent ndbroadbent deleted the nathan/security-fixes branch January 30, 2026 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ndbroadbent